Skip to content

🐛Ensure Dask client reference is uniquely defined for reference counting #7937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jun 20, 2025
Merged

🐛Ensure Dask client reference is uniquely defined for reference counting #7937

merged 2 commits into from
Jun 20, 2025

Conversation

@sanderegg
Copy link
Member

@sanderegg sanderegg commented Jun 19, 2025

What do these changes do?

A director-v2 replica keeps a dask-clients pool, each client is referenced counted since #7880.
The reference counting is using a reference that was defined using user_id and run_id. The run_id is an integer which does not ensure uniqueness. This means that some clients could maybe get deleted before the need. This would explain why restarting the director-v2 would fix it.

This PR fixes this issue by adding the project_uuid into the reference. Therefore it is now really unique.

Related issue/s

How to test

Dev-ops

@sanderegg sanderegg added this to the Engage milestone Jun 19, 2025
@sanderegg sanderegg self-assigned this Jun 19, 2025
@sanderegg sanderegg requested a review from pcrespov as a code owner June 19, 2025 17:07
@sanderegg sanderegg added the a:director-v2 issue related with the director-v2 service label Jun 19, 2025
@sanderegg sanderegg requested a review from GitHK as a code owner June 19, 2025 17:07
Copilot AI reviewed Jun 19, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes a bug in the Dask client reference counting by ensuring that the reference string is unique through the addition of a project identifier.

  • Updated the Dask client reference string to include "project_id".
  • Added "project_id" as a new parameter to relevant functions and updated client pool calls accordingly.
Comments suppressed due to low confidence (1)

services/director-v2/src/simcore_service_director_v2/modules/comp_scheduler/_scheduler_dask.py:248

  • The new formatting in _release_resources uses 'project_id' which is not defined in this scope. It should likely use 'comp_run.project_uuid' to ensure consistency with the other function calls.
                ref=_DASK_CLIENT_RUN_REF.format(

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 19, 2025

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@sanderegg sanderegg added the release Preparation for pre-release/release label Jun 19, 2025
@codecov
Copy link

codecov bot commented Jun 19, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.82%. Comparing base (a21f916) to head (e38545b).
Report is 1 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (a21f916) and HEAD (e38545b). Click for more details.

HEAD has 31 uploads less than BASE
Flag BASE (a21f916) HEAD (e38545b)
unittests 32 1
Additional details and impacted files 
@@             Coverage Diff             @@
##           master    #7937       +/-   ##
===========================================
- Coverage   88.02%   69.82%   -18.20%     
===========================================
  Files        1846      691     -1155     
  Lines       71224    32665    -38559     
  Branches     1220      176     -1044     
===========================================
- Hits        62694    22808    -39886     
- Misses       8179     9799     +1620     
+ Partials      351       58      -293
Flag Coverage Δ
integrationtests 64.18% <100.00%> (-0.03%) ⬇️
unittests 84.57% <100.00%> (-2.06%) ⬇️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library ∅ <ø> (∅)
pkg_dask_task_models_library ∅ <ø> (∅)
pkg_models_library ∅ <ø> (∅)
pkg_notifications_library ∅ <ø> (∅)
pkg_postgres_database ∅ <ø> (∅)
pkg_service_integration ∅ <ø> (∅)
pkg_service_library ∅ <ø> (∅)
pkg_settings_library ∅ <ø> (∅)
pkg_simcore_sdk 76.86% <ø> (-8.25%) ⬇️
agent ∅ <ø> (∅)
api_server ∅ <ø> (∅)
autoscaling ∅ <ø> (∅)
catalog ∅ <ø> (∅)
clusters_keeper ∅ <ø> (∅)
dask_sidecar ∅ <ø> (∅)
datcore_adapter ∅ <ø> (∅)
director ∅ <ø> (∅)
director_v2 91.11% <100.00%> (+0.02%) ⬆️
dynamic_scheduler ∅ <ø> (∅)
dynamic_sidecar 88.33% <ø> (-1.77%) ⬇️
efs_guardian ∅ <ø> (∅)
invitations ∅ <ø> (∅)
payments ∅ <ø> (∅)
resource_usage_tracker ∅ <ø> (∅)
storage ∅ <ø> (∅)
webclient ∅ <ø> (∅)
webserver 58.91% <ø> (-28.72%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a21f916...e38545b. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sanderegg sanderegg added the 🤖-automerge marks PR as ready to be merged for Mergify label Jun 19, 2025
@sanderegg
Copy link
Member Author

sanderegg commented Jun 19, 2025

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jun 19, 2025 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 11846e3

@mergify mergify bot merged commit 11846e3 into ITISFoundation:master Jun 20, 2025
245 of 255 checks passed
matusdrobuliak66 pushed a commit that referenced this pull request Jun 20, 2025
@sanderegg @matusdrobuliak66
🐛Ensure Dask client reference is uniquely defined for reference count…
8181b03 
…ing (#7937)
@YuryHrytsuk YuryHrytsuk mentioned this pull request Jun 20, 2025
Closed
15 tasks
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Jun 23, 2025
Closed
92 tasks
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
Closed
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@pcrespov pcrespov pcrespov approved these changes

@matusdrobuliak66 matusdrobuliak66 matusdrobuliak66 approved these changes

@GitHK GitHK Awaiting requested review from GitHK GitHK is a code owner

@giancarloromeo giancarloromeo Awaiting requested review from giancarloromeo

@bisgaard-itis bisgaard-itis Awaiting requested review from bisgaard-itis

Assignees

@sanderegg sanderegg

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:director-v2 issue related with the director-v2 service release Preparation for pre-release/release

Projects

None yet

Milestone

Engage

Development

Successfully merging this pull request may close these issues.

3 participants

@sanderegg @pcrespov @matusdrobuliak66